-
Couldn't load subscription status.
- Fork 1k
Deprecate DbClientCommonAttributesGetter #15139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| /** | ||
| * SqlClientAttributesExtractor will try to populate db.operation.name based on {@link | ||
| * #getRawQueryTexts(REQUEST)}, but this can be used to explicitly provide the operation name. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a copy-paste mistake
| /** | ||
| * SqlClientAttributesExtractor will try to populate db.operation.name based on {@link | ||
| * #getRawQueryTexts(REQUEST)}, but this can be used to explicitly provide the operation name. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a copy-paste mistake
| @Override | ||
| public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) { | ||
| super.onStart(attributes, parentContext, request); | ||
| // Common attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could consider placing the common part to util class or perhaps even a static package private method in DbClientAttributesExtractor that could also be called from here.
Semconv doesn't have this concept.
HttpCommonAttributesGetter makes sense b/c it's common attributes across both client and server.
But in semconv, Sql "extends" Db semantic conventions, and so I think our getter hierarchy should do the same.
In practice, this means that Sql getter now has things like getDbOperationName(), which I think is probably good, though we can default them to
nullin the Sql getter interface to make them not required.Related to #12608